fix(DataList): Padding on first column and alignment for headers#3277
fix(DataList): Padding on first column and alignment for headers#3277codecademydev merged 17 commits intomainfrom
Conversation
|
View your CI Pipeline Execution ↗ for commit d261d0c ☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3277 +/- ##
==========================================
- Coverage 89.77% 89.12% -0.65%
==========================================
Files 362 236 -126
Lines 5231 4342 -889
Branches 1696 1478 -218
==========================================
- Hits 4696 3870 -826
+ Misses 525 463 -62
+ Partials 10 9 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| : sortDirection === 'asc' | ||
| ? 'ascending' | ||
| : 'descending'; | ||
| {selectable && ( |
There was a problem hiding this comment.
removed the fragment b.c. it was interfering with the addition of data-first-col for the first element in the TableHeader
Also doesn't seem necessary
There was a problem hiding this comment.
Not using data-*-col approach anymore, so re-adding is fine, but it's also not necessary from what I can tell
LinKCoding
changed the title
packages/gamut/src/List/utils.ts
Outdated
| import { Children, cloneElement, isValidElement, ReactNode } from 'react'; | ||
|
|
||
| /** | ||
| * Marks the first and last columns with `data-first-col` / `data-last-col` |
There was a problem hiding this comment.
i really dislike cloning children, especially in a component like a DataList where we're already looping though all the columns to render them and there could be alot of them. maybe we you can move this logic into checking the idx of the React.Map in TableHeaderRow?
There was a problem hiding this comment.
FWIW, I did try this approach and it worked well for TableHeaderRow!
But when I went to see how we could use it for List, I ran into an issue again where the children are pretty configurable - i.e. I couldn't just add to the children's attribute without some type of new iteration.
And then after trying another approach with setting context and passing on the idx, the robot confirmed my suspicion that it's just as expensive as cloning.
So instead, I went back to the drawing board and realized that if we can't add padding to the children, maybe we can do that for the parent (row) instead. Since we only want either the first td or th — which seems like it'd translate to just the px of the row itself.
Looks like this works, and I've cleaned up some code that tries to be selective about what child gets padding. But yea, would compare against DataList, DataTable, and List.
dreamwasp
left a comment
dreamwasp
left a comment
There was a problem hiding this comment.
lgtm! if you have time could you remove the red BG on this List example? ik it is unrelated and literally probably something i did
i
|
📬 Published Alpha Packages: |
|
🚀 Styleguide deploy preview ready! Preview URL: https://69bc090ba811331719410540--gamut-preview.netlify.app |
3 participants
Overview
PR Checklist
Testing Instructions
Don't make me tap the sign.
8padding on the column's textpl: 8and the last element getspr:8(this can be done on one or many DataList)PR Links and Envs